-
Notifications
You must be signed in to change notification settings - Fork 19
👍 Add a priority property to Decoration #295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
👍 Add a priority property to Decoration #295
Conversation
WalkthroughAdds an optional per-decoration Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Buffer as buffer/decoration
participant Backend as Vim/Neovim API
Caller->>Buffer: decorate([{ highlight, priority?, ... }])
Note over Buffer: derive prop-type key = (prefix, priority, highlight)
alt prop-type missing
Buffer->>Backend: prop_type_add / prop_type_create (include priority)
end
Buffer->>Backend: prop_add_list / nvim_buf_set_extmark (attach decorations with priority)
Caller->>Buffer: listDecorations()
Backend-->>Buffer: prop_type_get / extmark info (includes priority)
Buffer-->>Caller: Decoration[] (each includes priority, default 0)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
buffer/decoration.ts (2)
38-43: Clarify JSDoc: negative values allowedCall out that negative priorities are allowed (per Vim’s behavior) to set expectations for consumers.
Apply this diff to the JSDoc:
- /** - * Priority for the decoration (higher numbers are displayed on top) - * Defaults to 0 if not specified - */ + /** + * Priority for the decoration (higher numbers are displayed on top). + * Negative values are allowed. Defaults to 0 when omitted. + */
260-276: Make Vim listing faster and more robust (fallback + cache + safe highlight parse)
- Avoid 1 RPC per prop by caching type->priority.
- Fallback to 0 when
prop_type_get()returns no priority.- Parse highlight without relying on
.split(':').pop()so it won’t break if a highlight ever contains a colon.Apply this diff:
- return Promise.all( - props - .filter((prop) => prop.type.startsWith(`${prefix}:`)) - .map(async (prop) => { - const propType = await vimFn.prop_type_get(denops, prop.type) as { - priority: number; - }; - return ({ - line: prop.lnum, - column: prop.col, - length: prop.length, - highlight: prop.type.split(":").pop() as string, - priority: propType.priority, - }); - }), - ); + const filtered = props.filter((prop) => prop.type.startsWith(`${prefix}:`)); + const typePriorityCache = new Map<string, number>(); + return Promise.all(filtered.map(async (prop) => { + let prio = typePriorityCache.get(prop.type); + if (prio == null) { + const t = await vimFn.prop_type_get(denops, prop.type) as { priority?: number }; + prio = t.priority ?? DEFAULT_DECORATION_PRIORITY; + typePriorityCache.set(prop.type, prio); + } + const head = `${prefix}:${prio}:`; + const highlight = prop.type.startsWith(head) + ? prop.type.slice(head.length) + : prop.type; // safe fallback + return { + line: prop.lnum, + column: prop.col, + length: prop.length, + highlight, + priority: prio, + }; + }));buffer/decoration_test.ts (1)
57-66: Avoid asserting internal Vim prop type namingAsserting the exact
typestring (including embedded priority) tightly couples tests to an internal naming scheme. Prefer verifying behavior vialistDecorationsor by checking theprioritythroughprop_type_get()for the relevant type(s).Example alternatives:
- Use
listDecorationsto assert highlight range and omit type internals.- Or, derive the prop type name, call
prop_type_get(), and assertpriority: 0.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
buffer/decoration.ts(7 hunks)buffer/decoration_test.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
buffer/decoration_test.ts (2)
function/buffer.ts (1)
bufnr(301-307)buffer/decoration.ts (2)
decorate(78-92)listDecorations(153-166)
buffer/decoration.ts (1)
batch/batch.ts (2)
batch(67-73)batch(187-203)
🔇 Additional comments (8)
buffer/decoration.ts (5)
12-12: Good default constantDEFAULT_DECORATION_PRIORITY = 0 aligns with Vim’s default behavior.
63-63: Nice exampleIncluding an explicit priority in the example helps demonstrate ordering semantics.
175-208: Vim prop types keyed by (priority, highlight): LGTMType naming and caching strategy make priority part of the prop-type identity; batch creation looks correct.
352-353: Default priority fallback (Neovim) looks goodSafe defaulting to 0 when the detail is missing.
298-299: Extmark priority supported since Neovim v0.7/API level 7
Thepriorityoption onnvim_buf_set_extmarkis available since API level 7 (Neovim ≥ 0.7), so no change is needed if your minimum supported version is v0.7 or newer.buffer/decoration_test.ts (3)
126-172: Priority coexistence test: LGTMValidates multiple decorations of the same highlight with different priorities across hosts.
185-215: listDecorations priority defaults and propagation: LGTMCovers default priority (0) and explicit priority (10).
230-247: Undecorate tests with priorities: LGTMBoth full-buffer and ranged clears behave correctly with prioritized decorations.
Also applies to: 262-287
|
I'm sorry but we first need to fix CI (it's our fault.) |
|
@blyoa Could you rebase from the latest main and force push? CI would be fixed (prob. CI fails to generate coverage report on Windows but we will ignore that.) |
2f703e0 to
664a39b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #295 +/- ##
==========================================
+ Coverage 84.77% 84.82% +0.04%
==========================================
Files 64 64
Lines 2864 2879 +15
Branches 277 281 +4
==========================================
+ Hits 2428 2442 +14
- Misses 434 435 +1
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@lambdalisue |
This Pull Request adds a
priorityproperty to theDecorationinterface, allowing users to control the display order of multiple decorations.The default priority is set to zero to align with the
priorityproperty ofprop_type_add.https://github.com/vim/vim/blob/fdeb721251481c26c230a0cd793cb89c2534c206/runtime/doc/textprop.txt#L426-L438
Summary by CodeRabbit
New Features
Tests